Acquisition Rate of planar mode is not used #145

fedem-p · 2022-07-27T13:08:43Z

Here's a simple fix to the second problem listed in #144.

The Issue

changing the parameter regarding the how many planes per second should be recorded doesn't update the saving parameter values

The top bar gui receive the info to display (number of volumes) from the save_status inside state.py
The save_status is changed in the stacksaver using the received save_params
the save_params is a property of the state which converts the appropriate settings in saving parameters

flowchart LR;
    stacksaver -->save_status;
    save_params -->stacksaver;
    save_status -->top_bar_gui;

    subgraph state
settings --> save_params;
    save_status;
    save_params;
    end 

    subgraph top_guy
    top_bar_gui;
    end

    subgraph stream_saver
    stacksaver;
    end

Now, the problem relies in the fact that save_params and the function convert_save_params only accepted/used ZRecordingSettings to extract the scanning_settings.
This works well for volumetric mode but fails to account for any changes in the single_plane_settings.

The Solution

I changed three functions:

save_params to determine whether it's planar or volumetric mode and pass to convert_save_params the appropriate settings (the default is volumetric)
convert_save_params to accept and correctly extract information from both volume_settings and single_plane_settings (in particular -> n_planes is set to 0 if in planar mode)
get_voxel_size to compute a voxel size even in planar_mode -> z voxel is left at 1

What's left

I'd like to hear some feedback regarding two main values:

n_planes which is set to 0 in planar_mode -> maybe it could be better to set it to None?
the default voxel size for the z axis in planar mode is set to 1 -> or maybe it should only return a tuple of 2 values if it's planar mode?
unfortunately this doesn't solve the first problem listed in Timing and other related bugs #144 which would now affect also the planar mode (probably better addressed in another PR)
write docstrings

fedem-p · 2022-07-27T13:11:37Z

sashimi/state.py

+    if isinstance(scanning_settings, SinglePlaneSettings):
+        inter_plane = 1
+    else:
+        scan_length = (
+            scanning_settings.piezo_scan_range[1]
+            - scanning_settings.piezo_scan_range[0]
+        )
+        inter_plane = scan_length / scanning_settings.n_planes


Inside get_voxel_size:
checks if the scanning_settings are single_plane_settings and if so sets the z_voxel size to 1

fedem-p · 2022-07-27T13:13:04Z

sashimi/state.py

-    n_planes = scanning_settings.n_planes - (
-        scanning_settings.n_skip_start + scanning_settings.n_skip_end
-    )
+    if isinstance(scanning_settings, SinglePlaneSettings):
+        n_planes = 0
+    else:
+        n_planes = scanning_settings.n_planes - (
+            scanning_settings.n_skip_start + scanning_settings.n_skip_end
+        )


Inside convert_save_params checks if the settings are for planar mode, and if so, sets n_planes to 0

fedem-p · 2022-07-27T13:13:58Z

sashimi/state.py

+        if self.global_state == GlobalState.PLANAR_PREVIEW:
+            save_p = convert_save_params(
+                self.save_settings,
+                self.single_plane_settings,
+                self.camera_settings,
+                self.trigger_settings,
+            )
+        else:
+            save_p = convert_save_params(
+                self.save_settings,
+                self.volume_setting,
+                self.camera_settings,
+                self.trigger_settings,
+            )
+        return save_p


inside save_params checks if we are in planar mode and returns the planar settings - if not defaults to volume settings

coveralls · 2022-07-27T13:16:06Z

Pull Request Test Coverage Report for Build 2746868032

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

For more information on this, see Tracking coverage changes with pull request builds.
To avoid this issue with future PRs, see these Recommended CI Configurations.
For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

9 of 12 (75.0%) changed or added relevant lines in 1 file are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.05%) to 79.798%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
sashimi/state.py	9	12	75.0%

Totals
Change from base Build 2744873404:	0.05%
Covered Lines:	2374
Relevant Lines:	2975

💛 - Coveralls

diegoasua · 2022-07-27T22:27:21Z

Let's keep n_planes as 0 so that there is no casting. And it is actually more informative. On the other hand Int to None is not exactly a casting operation but it could have some undesirable effects downstream. In particular, I am not 100% sure that the external_communication process will handle this correctly. It should. And testing should catch that otherwise. But certainty is not an exact science in Sashimi as you know. And overall it makes more sense to specify that there were 0 planes than None planes, as long as this is documented as being a single-plane recording. Same goes for voxel size, just setting to 1 is fine.

TLDR keep n_planes as 0

diegoasua · 2022-07-27T22:28:42Z

By the way that diagram is super cool. Which tool did you use?

vigji · 2022-07-28T07:35:29Z

I would say that the defaults you use would depend on the saving format.

If the output in planar mode is a (t, 1, w, h) stack, then n_planes should be 1 and voxel_size=(None, w_res, h_res)
If the output in planar mode is a (t, w, h) stack, then n_planes should be None/0 and voxel_size=(w_res, h_res)

fedem-p · 2022-07-28T08:04:13Z

By the way that diagram is super cool. Which tool did you use?

Thanks 👍 😄
I used a github codeblock with mermaid (I literally found it yesterday and is basically a languagge for writing flowcharts and other cool things) :)

nvbln · 2024-02-28T13:00:25Z

Since there have been no comments in the last two years and the PR seems to work I'll merge it.

save also single plane params

d749689

fedem-p added the bug Something isn't working label Jul 27, 2022

fedem-p self-assigned this Jul 27, 2022

fedem-p requested review from vigji, diegoasua and vilim July 27, 2022 13:09

fedem-p commented Jul 27, 2022

View reviewed changes

diegoasua approved these changes Jul 27, 2022

View reviewed changes

nvbln marked this pull request as ready for review February 28, 2024 11:54

hlavian force-pushed the time_bug branch from 6d3ea3c to d749689 Compare February 28, 2024 12:57

nvbln merged commit ee1fe72 into master Feb 28, 2024
4 of 6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Acquisition Rate of planar mode is not used #145

Acquisition Rate of planar mode is not used #145

fedem-p commented Jul 27, 2022 •

edited

Loading

fedem-p Jul 27, 2022

fedem-p Jul 27, 2022

fedem-p Jul 27, 2022

coveralls commented Jul 27, 2022 •

edited

Loading

diegoasua commented Jul 27, 2022

diegoasua commented Jul 27, 2022

vigji commented Jul 28, 2022

fedem-p commented Jul 28, 2022 •

edited

Loading

nvbln commented Feb 28, 2024

Acquisition Rate of planar mode is not used #145

Acquisition Rate of planar mode is not used #145

Conversation

fedem-p commented Jul 27, 2022 • edited Loading

The Issue

The Solution

What's left

fedem-p Jul 27, 2022

Choose a reason for hiding this comment

fedem-p Jul 27, 2022

Choose a reason for hiding this comment

fedem-p Jul 27, 2022

Choose a reason for hiding this comment

coveralls commented Jul 27, 2022 • edited Loading

Pull Request Test Coverage Report for Build 2746868032

Warning: This coverage report may be inaccurate.

Details

💛 - Coveralls

diegoasua commented Jul 27, 2022

diegoasua commented Jul 27, 2022

vigji commented Jul 28, 2022

fedem-p commented Jul 28, 2022 • edited Loading

nvbln commented Feb 28, 2024

fedem-p commented Jul 27, 2022 •

edited

Loading

coveralls commented Jul 27, 2022 •

edited

Loading

fedem-p commented Jul 28, 2022 •

edited

Loading